cc @bpaquet

Rebased and update to minio 7.0.97 removing a module importing a lot: minio/minio-go@v7.0.95...v7.0.97#diff-ca70b2f0fe15f1fa83771dd5b232679a5db6bd062df5cbd081a5cb14c00d87c8L14

@bpaquet I think we would like to get this in, unless you have some arguments against it.

cc @azr as well

I had a quick look, seems the client only support static creds. IMHO we should keep a way to use any kind of creds.
With a buildx scenario, the creds can be "resolved" by buildx, but without buildx, using minio-go seems to limit to static creds usage.

Standard creds are

• Ec2 Instance profile
• Web identify token (inside kube)
• Temporary creds through SSO

From a personal point of view, I think the standard is s3 :)

Question: why not make a minio remotecache ? and sort of slowly deprecate the s3 one, to give people options & time ? It would also make it easy to fix/circumvent any issues and still allow to avoid these pitfalls described up there.

Other than that, I don't think I can come up with any strong objection with this.

+1, seems better to do a minio remote cache exporter and mutualize the code

Thanks for the review and for flagging the credential concerns, @bpaquet 🙏

I had a quick look, seems the client only support static creds. IMHO we should keep a way to use any kind of creds. With a buildx scenario, the creds can be "resolved" by buildx, but without buildx, using minio-go seems to limit to static creds usage._

You’re right that the current diff effectively narrows us to static keys. I’m proposing to wire minio-go’s credential chain so we keep parity with common setups (env, shared file, and IAM/IRSA/ECS/IMDS), while preserving explicit static keys as today.

Tiny, backward-compatible change:

func resolveCredentials(config Config) *credentials.Credentials {
    if config.AccessKeyID != "" || config.SecretAccessKey != "" || config.SessionToken != "" {
        return credentials.NewStaticV4(config.AccessKeyID, config.SecretAccessKey, config.SessionToken)
    }

    return credentials.NewChainCredentials([]credentials.Provider{
        &credentials.EnvAWS{},             // AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY / AWS_SESSION_TOKEN
        &credentials.EnvMinio{},           // MINIO_ACCESS_KEY / MINIO_SECRET_KEY / MINIO_SESSION_TOKEN
        &credentials.FileAWSCredentials{}, // ~/.aws/credentials or AWS_SHARED_CREDENTIALS_FILE (+ AWS_PROFILE)
        &credentials.IAM{Client: &http.Client{}}, // EC2 IMDS / ECS / IRSA (web identity)
    })
}

… and then use it when creating the client:

client, err := minio.New(parsedURL.Host, &minio.Options{
    Creds:        resolveCredentials(config),
    Secure:       parsedURL.Scheme == "https",
    Region:       config.Region,
    BucketLookup: bucketLookup,
})

+1, seems better to do a minio remote cache exporter and mutualize the code

Yes, that is of course also a option. Should I create a new pull request with the new cache provider?

+1, seems better to do a minio remote cache exporter and mutualize the code

Yes, that is of course also a option. Should I create a new pull request with the new cache provider?

Don't think we want another cache exporter that would increase the binary size with new set of dependencies if there is no real adding-value.

Tiny, backward-compatible change:

func resolveCredentials(config Config) *credentials.Credentials {
if config.AccessKeyID != "" || config.SecretAccessKey != "" || config.SessionToken != "" {
return credentials.NewStaticV4(config.AccessKeyID, config.SecretAccessKey, config.SessionToken)
}

return credentials.NewChainCredentials([]credentials.Provider{
    &credentials.EnvAWS{},             // AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY / AWS_SESSION_TOKEN
    &credentials.EnvMinio{},           // MINIO_ACCESS_KEY / MINIO_SECRET_KEY / MINIO_SESSION_TOKEN
    &credentials.FileAWSCredentials{}, // ~/.aws/credentials or AWS_SHARED_CREDENTIALS_FILE (+ AWS_PROFILE)
    &credentials.IAM{Client: &http.Client{}}, // EC2 IMDS / ECS / IRSA (web identity)
})

}

… and then use it when creating the client:

client, err := minio.New(parsedURL.Host, &minio.Options{
Creds: resolveCredentials(config),
Secure: parsedURL.Scheme == "https",
Region: config.Region,
BucketLookup: bucketLookup,
})

Thanks @sorenhansendk, I think using a default set of credential providers looks good if config.AccessKeyID and config.SecretAccessKey are not set.

Don't think we want another cache exporter that would increase the binary size with new set of dependencies if there is no real adding-value.

In the use cases I've seen, binary size really doesn't matter as the image would be cached, plus bandwidth and storage are cheap, etc. and this wouldn't be such a big increase. Also, this would be temporary, until we remove the s3 lib. Moreover, isn't the s3 lib used elsewhere ?

The annoying cost would be a maintain cost, I think, and there I hear you, that's a bigger burden than it should.

The added value would be the stability here, I don't know that there is a bug but a few discrepancies were found already, etc.